-
Notifications
You must be signed in to change notification settings - Fork 70
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(stream): sender refactor with rate enforcement #616
Conversation
c740d7c
to
958c80d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Rates crate looks good
- Service-util refactor also looks good
- great work on the new stream client, finally it will not infinitely loop if we send a payment above min_amount
Here's a PR with some more suggestions: #629
crates/interledger-stream/src/lib.rs
Outdated
assert!( | ||
match result { | ||
Err(Error::SendMoneyError(_)) => true, | ||
_ => false, | ||
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: You could just do let err = send_money(...).await.unwrap_err()
, and then check that the error is the expected one
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I couldn't find an idiomatic way to compare enum variants without a match? And then it's kinda awkward to translate that into an assertion
PaymentEvent::MaxInFlight => { | ||
// Wait for 100ms for any request to complete, otherwise try running loop again | ||
// to see if we reached the timeout since last fulfill | ||
let fut = timeout( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
technically since you called await
on this, it's no longer a future but the inner result. It might be useful to make this timeout be an exponential backoff or grow in some way? Maybe add a TODO if you think it'd be useful, otherwise leave as is
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated this so it calculates the deadline (last fulfill timestamp + max duration since last fulfill), then waits for any request to complete or reach the deadline before running the loop again, which should timeout the payment.
Co-Authored-By: Georgios Konstantopoulos <me@gakonst.com>
f17616f
to
70fcb21
Compare
Big things
interledger-rates
crateLittle things
/payments
endpoint (percentage of calculated exchange rate which is acceptable to deliver)